Optimise Iterator::{max, max_by, min, min_by}. #24180
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
| mut f_cmp: FCmp) -> Option<(B, I::Item)> | ||
| where I: Iterator, | ||
| FProj: FnMut(&I::Item) -> B, | ||
| FCmp: FnMut(&B, &I::Item, &B, &I::Item) -> bool |
There was a problem hiding this comment.
Is there some reason the comparison function takes 4 arguments, when each client only wants 2 of them, instead of just making it take the projections and having max() / min() use |x| x for the projection function?
There was a problem hiding this comment.
I agree it would be nice but the lifetimes don't work out, B is disconnected from &I::Item. I think the fact we move sel and x around internally means that this approach can't work. :(
|
r=me with nits addressed, nice wins! |
The main change in this patch is removing the use of `Option` inside the
inner loops of those functions to avoid comparisons where one branch
will only trigger on the first pass through the loop.
The included benchmarks go from:
test bench_max ... bench: 372 ns/iter (+/- 118)
test bench_max_by ... bench: 428 ns/iter (+/- 33)
test bench_max_by2 ... bench: 7128 ns/iter (+/- 326)
to:
test bench_max ... bench: 317 ns/iter (+/- 64)
test bench_max_by ... bench: 356 ns/iter (+/- 270)
test bench_max_by2 ... bench: 1387 ns/iter (+/- 183)
Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
|
@bors r=alexcrichton |
|
📌 Commit c2258d6 has been approved by |
|
⌛ Testing commit c2258d6 with merge f4aa02e... |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Apr 10, 2015
The main change in this patch is removing the use of `Option` inside the
inner loops of those functions to avoid comparisons where one branch
will only trigger on the first pass through the loop.
The included benchmarks go from:
test bench_max ... bench: 372 ns/iter (+/- 118)
test bench_max_by ... bench: 428 ns/iter (+/- 33)
test bench_max_by2 ... bench: 7128 ns/iter (+/- 326)
to:
test bench_max ... bench: 317 ns/iter (+/- 64)
test bench_max_by ... bench: 356 ns/iter (+/- 270)
test bench_max_by2 ... bench: 1387 ns/iter (+/- 183)
Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit c2258d6 with merge 1302028... |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Apr 10, 2015
The main change in this patch is removing the use of `Option` inside the
inner loops of those functions to avoid comparisons where one branch
will only trigger on the first pass through the loop.
The included benchmarks go from:
test bench_max ... bench: 372 ns/iter (+/- 118)
test bench_max_by ... bench: 428 ns/iter (+/- 33)
test bench_max_by2 ... bench: 7128 ns/iter (+/- 326)
to:
test bench_max ... bench: 317 ns/iter (+/- 64)
test bench_max_by ... bench: 356 ns/iter (+/- 270)
test bench_max_by2 ... bench: 1387 ns/iter (+/- 183)
Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
|
⛄ The build was interrupted to prioritize another pull request. |
bors
added a commit
that referenced
this pull request
Apr 10, 2015
The main change in this patch is removing the use of `Option` inside the
inner loops of those functions to avoid comparisons where one branch
will only trigger on the first pass through the loop.
The included benchmarks go from:
test bench_max ... bench: 372 ns/iter (+/- 118)
test bench_max_by ... bench: 428 ns/iter (+/- 33)
test bench_max_by2 ... bench: 7128 ns/iter (+/- 326)
to:
test bench_max ... bench: 317 ns/iter (+/- 64)
test bench_max_by ... bench: 356 ns/iter (+/- 270)
test bench_max_by2 ... bench: 1387 ns/iter (+/- 183)
Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main change in this patch is removing the use of
Optioninside theinner loops of those functions to avoid comparisons where one branch
will only trigger on the first pass through the loop.
The included benchmarks go from:
to:
Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/